Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get rid of csp-module and implement secure-renderer #79

Closed
wants to merge 1 commit into from

Conversation

rommelfreddy
Copy link
Contributor

hello @jissereitsma

you used your CSP module to implement the nonce for inline-js.

But as already discussed yireo/Yireo_CspWhitelistInlineJs#1 the module is great, but it is only a temporary solution for production.

Each extension developer should implement the secure-renderer to use inline-scripts. also Yireo :)

And then we do not need the separate extension. I don't think it is a good idea to implement the separate extension in all shops automatically, because some shops won't have the extension.

So i hope you understand me, and will approve the PR :)

Thank you!

@jissereitsma
Copy link
Contributor

I understand your points ... partially.

First of all, my change in these modules (including the CspUtilities module) has nothing to do with the CspWhitelistInlineJs module - except for that a lot of my other modules (CspWhitelistInlineJs, GoogleTagManager, NextGenImages, Webp and perhaps more in the future) are including the CspUtilities module as a dependency. Your reference to the discussion of the security issues of CspWhitelistInlineJs do not apply here. The CspWhitelistInlineJs module explicitly allows all scripts, while the other modules explicitly allow their own scripts (as your PR does as well).

The actual question is why I'm not using the $secureRenderer as proposed in your PR. The reason for this is very simple: The $secureRenderer variable was added to PHTML templates as per Magento 2.4.x and does not work in older versions like 2.3.7 even though the CSP module was already there. In other words, supporting CSP in the way you propose will drop backwards compatibility with older Magento versions, even though they would have been patched to the latest security patches. Unfortunately, my modules are used by a lot of shops that still run older versions of Magento and I need to be backwards compatible, or release a new major module version here (which is a challenge on its own).

Your PR still misses the composer dependency

To remain backwards compatible I've struggled with creating proxies, factories and other tricks for the SecureHtmlRenderer class but unfortunately this all leads into issues (mostly weird loading issues in Production Mode). Using the CSP nonce generator class directly makes much more sense. And note that the usage of $secureRenderer is not the only solution recommended by Magento, using the CSP nonce generator is equally as good - and they both boil down to the same thing: Generating nonces. It's just that the CSP nonce generator allows for a solution that is backwards compatible and the SecureHtmlRenderer does not. Your statement "Each extension developer should implement the secure-renderer to use inline-scripts" is definitely not correct. To my opinion, the statement should be "Each extension developer should support CSP, including their inline-scripts".

Furthermore, your PR includes the original script in a HEREDOC. This solution requires a lot of rewriting of scripts, is harder to read and (!) create issues in specific Magento versions (yireo/Yireo_GoogleTagManager2#231). Not using a HEREDOC syntax but just adding the nonce after rendering the script makes more sense to me. And that's what the CspUtilities module does.

This is all explaining why $secureRenderer is not the best of solutions. It does not explain yet why I introduced the CspUtilities module in the first place.

I don't think it is a good idea to implement the separate extension in all shops automatically, because some shops won't have the extension.

Note that the usage of composer will sort out the dependencies just fine, so shops that are using composer don't need to worry a thing. As of yet, the CspUtilities module offers utility classes, nothing more. There is not even a strong requirement to enable the module, even though it is a module. However, by moving this specific problem into a seperate module I'm following best practices of Magento (creating submodules for subproblems), avoiding code duplication (mess detector will not complain) and fix possible future problems for all my modules at once.

As of yet, I'm inclined to close your PR because of this. I'll keep it open for discussion. However, if - one day - I'm going to drop support for older versions of Magento, I will definitely drop the additional CspUtility module as well. However, using the $secureRenderer will not be an option for me, I'd rather choose the documented option for injecting a ViewModel that againd depends upon the CSP nonce generator.

@jissereitsma
Copy link
Contributor

I'm closing this as of yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants